[AutoBuild] Replaced platform triplet string comparison with platform comparison in rebuild_jll_package#1363
Conversation
… comparison in rebuild_jll_package
giordano
left a comment
giordano
left a comment
There was a problem hiding this comment.
I haven't looked at the code very closely yet, but at a high level this looks very promising, thanks for doing this.
Might be an idea to look into a test or two…
Yes, the registration pipeline is largely untested here, because it's also complicated to reproduce end-to-end without a mock registry and such. However this is a small self-contained function which should be easy to unit test, so please do go ahead and add tests!
src/AutoBuild.jl
Outdated
| end | ||
| try | ||
| tarball_filename_platform = parse(Platform, tarball_filename_match[:platform_triplet]) | ||
| return tarball_filename_platform == platform |
There was a problem hiding this comment.
One thing I'm not 100% sure is whether this comparison is fine. Platform equality is finicky, I usually prefer using platform_match, but probably in this case we really want to check exact equality specifically, so this may be good.
There was a problem hiding this comment.
Good points - both of them. I’ll check up on platform_match.
There was a problem hiding this comment.
julia> using Base.BinaryPlatforms
julia> p1 = Platform("x86_64", "linux"; tag1="foo", tag3="blah")
Linux x86_64 {libc=glibc, tag1=foo, tag3=blah}
julia> p2 = Platform("x86_64", "linux"; tag2="bar", tag4="glah")
Linux x86_64 {libc=glibc, tag2=bar, tag4=glah}
julia> p1 == p2
false
julia> platforms_match(p1, p2)
true
julia> p3 = Platform("x86_64", "linux"; tag3="blah", tag1="foo")
Linux x86_64 {libc=glibc, tag1=foo, tag3=blah}
julia> p4 = Platform("x86_64", "linux"; tag4="glah", tag2="bar")
Linux x86_64 {libc=glibc, tag2=bar, tag4=glah}
julia> platforms_match(p1, p3)
true
julia> p1 == p3
true
julia> p2 == p4
true
Probably equality is better than platform_match here, but make sure with appropriate tests that it works correctly also in complex cases.
There was a problem hiding this comment.
I have added some tests.
I would argue that the functionality of the Platform constructor should not be tested here (p1 vs p3, and p2 vs p4), but the difference between Platform equality and platforms_match should, e.g. where platforms_match is true and equality is false,
julia> p5 = Platform("x86_64", "linux")
Linux x86_64 {libc=glibc}
julia> p6 = Platform("x86_64", "linux"; cxxstring_abi="cxx11")
Linux x86_64 {cxxstring_abi=cxx11, libc=glibc}
julia> platforms_match(p5, p6)
true
julia> p5 == p6
false
|
It should be noted that I did not get concrete evidence that this is indeed the root cause for the issues in JuliaPackaging/Yggdrasil#10426 etc., but it seems like a (very) likely cause. The issue with reproducation was primarily that With some quasi-empty tarballs in "./foo": platforms = expand_cxxstring_abis([
Platform("aarch64", "linux"; cuda="10.2", cuda_platform="jetson"),
Platform("x86_64", "linux"; cuda="11.3"),
Platform("x86_64", "windows"; cuda="11.3")
]; skip=!Sys.islinux)
# 5-element Vector{Platform}:
# Linux aarch64 {cuda=10.2, cuda_platform=jetson, cxxstring_abi=cxx03, libc=glibc}
# Linux aarch64 {cuda=10.2, cuda_platform=jetson, cxxstring_abi=cxx11, libc=glibc}
# Linux x86_64 {cuda=11.3, cxxstring_abi=cxx03, libc=glibc}
# Linux x86_64 {cuda=11.3, cxxstring_abi=cxx11, libc=glibc}
# Windows x86_64 {cuda=11.3}
readdir(joinpath(pwd(), "foo"))
# ONNXRuntime_CUDA.v1.10.0.aarch64-linux-gnu-cxx03-cuda_platform+jetson-cuda+10.2.tar.gz
# ONNXRuntime_CUDA.v1.10.0.aarch64-linux-gnu-cxx11-cuda_platform+jetson-cuda+10.2.tar.gz
# ONNXRuntime_CUDA.v1.10.0.x86_64-linux-gnu-cxx03-cuda+11.3.tar.gz
# ONNXRuntime_CUDA.v1.10.0.x86_64-linux-gnu-cxx11-cuda+11.3.tar.gz
# ONNXRuntime_CUDA.v1.10.0.x86_64-w64-mingw32-cuda+11.3.tar.gz
triplet(first(platforms))
# "aarch64-linux-gnu-cxx03-cuda_platform+jetson-cuda+10.2"
BinaryBuilder.rebuild_jll_package("foo", v"0.1.0", [], platforms, [], [], joinpath(pwd(), "foo"), "foo"; verbose=true)
# Runs past finding platform for each file in "./foo"I have also not discovered why the triplets sometimes had the |
|
Is this ready?
|
|
OK. Yes, I think it is ready. |
2 participants
Should remedy issues seen with registration of ONNXRuntime_CUDA (using the
cuda_platformplatform tag, e.g.,cuda_platform="jetson") in JuliaPackaging/Yggdrasil#10426, and investigated in JuliaPackaging/Yggdrasil#10476 .